Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'REDISTRIBUTE KEY RANGE' command #789

Open
wants to merge 52 commits into
base: master
Choose a base branch
from

Conversation

EinKrebs
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@Denchick Denchick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

  • Please add a description to all methods in balancer.go and coordinator.go
  • Please add TODOs to all new methods that you add
  • Please add regress tests for redistribute commond (just to check syntax and output)
  • Please add as much documentation as possible
  • I fount a lot of panics in code. You should assume that a panic will be immediately fatal, for the entire program, or at the very least for the current goroutine. Ask yourself "when this happens, should the application immediately crash?" If yes, use a panic; otherwise, use an error.

func (rel *DistributedRelation) GetDistributionKeyColumns() []string {
res := make([]string, len(rel.DistributionKey))
for i, col := range rel.DistributionKey {
// TODO: add hash
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to fix todo and add unittests here


for _, tt := range []tcase{
{
query: "REDISTRIBUTE KEY RANGE kr1 TO sh2 BATCH SIZE 500",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets add more corner cases like

  • batch size in not specified
  • batch size is negative
  • what is maximum supported batch size value? Minimum?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the purpose of this test, it only tests parser, thus will parse any integer constant

@@ -846,6 +854,12 @@ move_key_range_stmt:
$$ = &MoveKeyRange{KeyRangeID: $2.KeyRangeID, DestShardID: $4}
}

redistribute_key_range_stmt:
REDISTRIBUTE key_range_stmt TO any_id BATCH SIZE any_uint
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cool to allow REDISTRIBUTE without batch size, using some default.

@@ -679,7 +679,7 @@ func (tctx *testContext) stepWaitPostgresqlToRespond(host string) error {
const trials = 10
const timeout = 20 * time.Second
for i := 0; i < trials; i++ {
_, err := tctx.queryPostgresql(host, "SELECT 1", struct{}{})
_, err := tctx.queryPostgresql(host, "SELECT 1", struct{}{}, postgresqlQueryTimeout)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postgresqlQueryTimeout -> timeout like in 394 line

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need it here? Here the default timeout is sufficient

balancer/provider/balancer.go Show resolved Hide resolved
coordinator/provider/coordinator.go Show resolved Hide resolved
pkg/clientinteractor/interactor.go Outdated Show resolved Hide resolved
pkg/clientinteractor/interactor.go Show resolved Hide resolved
pkg/clientinteractor/interactor.go Outdated Show resolved Hide resolved
@@ -97,3 +100,30 @@ func (s *ShardConnect) GetConnStrings() []string {
}
return res
}

func (s *ShardConnect) GetConnectionPreferReplica(ctx context.Context) (*pgx.Conn, error) {
connStrings := s.GetConnStrings()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's at least leave a few tasks here, because ideally we need to make sure that this replica is functional enough.

For example, replication lag is less than value from config or we have metrics from host or maybe smth else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants